Use evaluator for isolatedModules enum restrictions #57966
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
The worst PR race; we just merged #57955 which moved this code. |
|
@typescript-bot test it |
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
Uh oh, the last merge just broke the PR a lot |
|
Yeah I was not on the branch I thought I was on 🤦🏻♂️ |
andrewbranch
force-pushed
the
bug/isolated-modules-enum-restrictions
branch
from
2d4e834 to
2035fc6
Compare
|
Hey @andrewbranch, the results of running the DT tests are ready. Everything looks the same! |
|
@andrewbranch Here are the results of running the user tests comparing Everything looks good! |
|
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -5626,6 +5626,7 @@ export interface EmitResolver { | |||
| isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult; | |||
| // Returns the constant value this property access resolves to, or 'undefined' for a non-constant | |||
| getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): string | number | undefined; | |||
There was a problem hiding this comment.
Does this still need to have EnumMember?
There was a problem hiding this comment.
Probably not, but the same signature is public API on TypeChecker so maybe it’s best not to change it?
There was a problem hiding this comment.
Actually, declarations.ts still calls getConstantValue but only on an enum value, which makes me think that the resolver wouldn't need that func at all anymore. But, ts.ts's tryGetConstEnumValue still calls it for property access / element accesses :(
|
@andrewbranch Here are the results of running the top 400 repos comparing Everything looks good! |
| @@ -167,6 +167,8 @@ import { | |||
| equateValues, | |||
| escapeLeadingUnderscores, | |||
| escapeString, | |||
| type EvaluatorResult, | |||
There was a problem hiding this comment.
This is a nit but we haven't really used type imports anywhere else quite yet (besides that protocol.ts blunder); I don't think we've set the settings in settings.json yet that would allow sorting to be inline either... Maybe we just avoid type imports until we can switch them all?
There was a problem hiding this comment.
Fine with me, but this is what auto-imports did automatically 😬 My bad, that’s my own setting; I guess I was testing it a while back. It works!
There was a problem hiding this comment.
This seems good to me, though I'm still hoping that we can reduce the EmitResolver API surface since there are so few calls to the old API left...
Maybe @weswigham @dragomirtitian have some feedback on that front just from needing to implement that resolver in a minimal fashion, but given this code all exists and works via the existing evaluator that was just pulled out, I doubt this is really a big deal.
This looks ok to me from the point of view of creating a minimalist resolver. |
With #57914, it becomes possible to enable
isolatedModulesin our own codebase. I tried this, but there were a few inconvenient changes required to enum declarations due to #56736. You can see them here: https://github.com/andrewbranch/TypeScript/compare/bug/37774...andrewbranch:TypeScript:enable-isolatedModules?expand=1The checks added in #56736 seem to be a bit too restrictive. Specifically, references to same-file constants and other enum members were not recognized. I refactored the evaluator to return extra information that lets us produce the same errors with higher fidelity to the capabilities of the evaluator. It seems that transpilers (tested esbuild and Babel) have built their behavior around what our own evaluator can do; they’re just obviously limited to analyzing a single file (when not bundling). So, this PR removes
isolatedModuleserrors from a lot of code that transpiles perfectly fine.(cc @frigus02)